fix(monitor): use IsValidUUID to skip uninitialised device UUIDs in scrape path - #2465
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change strengthens UUID validation for vGPU devices. Container metric collection skips uninitialized UUIDs, truncates valid UUIDs to 40 characters, and rejects invalid UTF-8. Tests now cover zero-filled UUIDs. ChangesUUID validation and metrics
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/vGPUmonitor/feedback_test.go`:
- Around line 67-69: Update stubInfo.IsValidUUID to validate that i is
non-negative before evaluating len(s.uuids[i]) or accessing s.uuids[i], while
preserving the existing bounds and UUID-content checks for valid indexes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 113379ac-1839-4c84-a79f-02221f1e33ce
📒 Files selected for processing (3)
cmd/vGPUmonitor/feedback_test.gocmd/vGPUmonitor/metrics.gocmd/vGPUmonitor/metrics_container_test.go
…crape path DeviceUUID always returns a 96-byte string (from [96]byte), so the prior guard len(uuid) < 40 could never fire. Containers starting up whose UUID has not yet been written by libvgpu passed through with 40 null bytes emitted as a Prometheus label value. Replace the dead length check with IsValidUUID, which tests uuid[0] != 0 and correctly detects uninitialized slots. Update stubInfo.IsValidUUID in the test helper to match the same semantics, and replace the short-UUID test case with an uninitialized-UUID case that reflects the real scenario. Signed-off-by: Nakshatra Sharma <nakshatra.sharma3012@gmail.com>
3adf9b0 to
07210ff
Compare
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
|
/lgtm |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: FouoF, Nakshatra480 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@FouoF Thanks for merging this pr 😊 |
3 participants
Summary
collectContainerMetricsguarded against uninitialized device UUIDs withlen(uuid) < 40. This check is dead code:DeviceUUID()returnsstring(s.sr.uuids[i].uuid[:])whereuuidis[96]byte, solenisalways 96 and the condition never fires.
The consequence: a container that is starting up, whose UUID slot has not yet
been written by libvgpu, passes through the guard with 40 null bytes emitted
as a Prometheus
device_uuidlabel value instead of being skipped.IsValidUUID(i)already exists in theUsageInfointerface and checksuuid[0] != 0- the correct "has libvgpu written this slot?" test. It isalready used this way in
feedback.go(line 86). This PR applies the samepattern to the scrape path.
Changes:
cmd/vGPUmonitor/metrics.go: replacelen(uuid) < 40with!c.Info.IsValidUUID(i); move the check beforeDeviceUUIDto matchfeedback.goand avoid a string allocation on uninitialized slots;collapse
uuid := c.Info.DeviceUUID(i)[0:40]into one statementcmd/vGPUmonitor/feedback_test.go: updatestubInfo.IsValidUUIDfromi < len(s.uuids)(always true within the loop) to checkuuid[0] != 0,matching the real implementation
cmd/vGPUmonitor/metrics_container_test.go: replace the "short UUID" testcase (tests a scenario that cannot occur in production since
DeviceUUIDalways returns 96 bytes) with an uninitialized-UUID case (null first byte)
that actually exercises the guard
Which issue(s) this PR fixes:
Part of #2126 (LFX observability hardening - Prometheus metric label correctness)
Special notes for your reviewer:
PR #2364 changed
return→continuefor this check, but the check itselfwas always unreachable. This PR makes the guard fire for the case it was
written for. The
utf8.ValidStringcheck underneath is left unchanged.Does this PR introduce a user-facing change?
Yes containers in the startup phase no longer emit metrics with null-byte
device_uuidlabels; those device slots are silently skipped until libvgpuwrites a valid UUID.
AI Disclosure:
AI assistance was used for code inspection and draft formatting; all logic,
test cases, and verification were manually reviewed and validated.
Summary by CodeRabbit